Separate computed and secret dependencies #123
Conversation
iwahbe
force-pushed
the
iwahbe/122/seperate-field-dependencies
branch
7 times, most recently
from
834ba2d to
a19cccb
Compare
iwahbe
force-pushed
the
iwahbe/122/seperate-field-dependencies
branch
from
a19cccb to
0cbf9d6
Compare
| @@ -207,9 +207,16 @@ type OutputField interface { | |||
| type InputField interface { | |||
| // Seal the interface. | |||
| isInputField() | |||
|
|
|||
| // Only wire secretness, not computedness. | |||
There was a problem hiding this comment.
This strikes me as very cryptic, can we unpack a bit?
| } else if !ende.DeepEquals( | ||
| r.NewObjectProperty(oldInput), | ||
| r.NewObjectProperty(newInput)) { | ||
| // If there is a change, then every item item should be |
There was a problem hiding this comment.
Hmm, why is this? This is simulating preview only I guess but still? It's a little interesting, trying to wrap my head around. So the implementation is allowed to copy oldInput to output, or mark outputs computed.
There was a problem hiding this comment.
The default logic is: "if any input property has changed, declare all the output properties as unknown computed, except the outputs that are just pass-through inputs". We mark all output properties as unknown because we don't know if the changed input will change the output. We do know if passthrough inputs are known.
There was a problem hiding this comment.
In this condition:
if n, ok := newInput[k]; !ok || ende.IsComputed(n) {If newInput[k] makes ok==false, then there are no asserts performed at all.
So I don't think the property you state in tested here. But implementation might still do it right! It's just not tested seemingly, or I don't get something.
Consider oldInput={"a": 1}, newInput={"a": 42}, output: {"a": 42, "b": "foo"}.
This is trying to simulate "a" input changing, I think this will pass the check although "b": "foo" didn't get marked unknown. By your logic above it seems that "b" should be unknown since we can't assume that pushing changed inputs through the resource black box won't change the outputs.
There was a problem hiding this comment.
I think you're right that the test is missing something. c8c23a0 strengthens the test to assert our expected conditions:
pulumi-go-provider/infer/resource_test.go
Lines 99 to 113 in c8c23a0
|
I'll go read the ticket again #122 |
|
The ticket makes sense #122 spec-wise actually I need some more time to work through the PR to grok it. |
|
Don't want to block the Command P1 fix. I can't find anything wrong with it but I'm still a bit fluffy on the verifying the implementation, if not for the time crunch I'd like to go more thorough on clarifying the by-design behavior especially since secret / unknown inference through black boxes is something we do and want to do across many providers (definitely all bridged providers!) it's an area worth getting rock-solid some time. |
I would love to work with you on nailing the semantics here, and then building a library that works the way we want it to, both here and in the bridge. |
Fixes #122
This PR separates dependency flow for computedness and secretness.
By default, we apply the following strategies:
Secrets
An output property is secret if and only if there is an input property of the same name that is also secret
Computed
An output property is computed if there is any change in between old and new inputs unless there is an input property of the same name and value that is not computed.